ref(project-state): Introduce a dedicated state for 'new allowed'#5998
ref(project-state): Introduce a dedicated state for 'new allowed'#5998Dav1dde wants to merge 2 commits into
Conversation
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
LGTM. Am I understanding correctly that it would be nicer to send no project config, rather than one with all fields empty/defaulted, but this is left as future work?
| // | ||
| // To support this, the proxy Relay would have to act as a pure proxy and not fetch | ||
| // the project configuration from its own cache. | ||
| continue; |
There was a problem hiding this comment.
Would it make sense to log a warning here?
There was a problem hiding this comment.
I decided against it, because it would log warnings on the "correctly" setup proxy Relay, instead of the incorrectly setup managed Relay.
Meaning someone could generate these warnings from the outside maliciously and they wouldn't be actionable for the user running the proxy Relay.
But maybe I am also overthinking it.
| macro_rules! pop_envelope { | ||
| () => {{ | ||
| relay_log::trace!("EnvelopeBufferService: popping envelope"); | ||
| // If we arrived here, know that both projects are available, so we pop the envelope. | ||
| // | ||
| // Available, doesn't necessarily mean enabled/active. | ||
| let envelope = buffer.pop().await?; | ||
| let envelope = envelope.expect("Element disappeared despite exclusive excess"); | ||
| Managed::from_envelope(envelope, services.outcome_aggregator.clone()) | ||
| }}; | ||
| } |
There was a problem hiding this comment.
Any reason this is a macro and not an async closure?
There was a problem hiding this comment.
Afaik I ran into problems with the closure borrowing parts mutably, but I'll double check.
| // If the own project state is disabled, we want to drop the envelope and early return since | ||
| // we can't do much about it. |
There was a problem hiding this comment.
I know this is an old comment but it sounds unnecessarily hand-wavy.
| // If the own project state is disabled, we want to drop the envelope and early return since | |
| // we can't do much about it. | |
| // If the own project state is disabled, we want to drop the envelope. |
| sampling_project_info, | ||
| } => { | ||
| let mut envelope = pop_envelope!(); | ||
| if own_project.check_envelope(&mut envelope).await.is_err() || envelope.is_empty() { |
There was a problem hiding this comment.
Could we move the call to check_envelope into resolve_project? Then you wouldn't have to return both Project and ProjectInfo from that function (though you'd still need the rate_limits).
| match self { | ||
| Self::Enabled(info) => Some(info), | ||
| Self::Disabled | Self::Pending => None, | ||
| Self::DummyAllowed | Self::Disabled | Self::Pending => None, |
There was a problem hiding this comment.
I vote to remove the .enabled() function entirely. The few places that use it now gloss over the fact that we forgot about proxy mode in those places (as you pointed out in the PR description). So we could replace those calls with explicit match statements and comment on the flawed proxy handling.
| // Since downstream requires a project config, we re-use this dummy config. | ||
| // | ||
| // This is how Relay historically always handled its proxy mode. | ||
| // It would make sense to instead of passing down this dummy, making the project | ||
| // config state here optional or similarly typed to the project state. |
There was a problem hiding this comment.
This is indeed unfortunate. Maybe in a follow-up we could have an EnabledProject enum that we pass into ProcessEnvelope:
enum ProjectState {
Enabled(EnabledProject),
Disabled,
}
enum EnabledProject { // This goes into the processor
Loaded(Arc<ProjectInfo>),
Dummy,
}That would also help with special-casing proxy mode in the processor, i.e. instead of checking if mode == RelayMode::Proxy, match on the EnabledProject. That would also force authors of processing code to think about proxy mode.
There was a problem hiding this comment.
Yeah I think that's a good idea.
That would also help with special-casing proxy mode in the processor [..]
Since we already have 2 separate processor implementations, this wouldn't even be annoying, the proxy processor could ignore it, the non-proxy one would "assert" it's Enabled.
| /// This is used by proxy Relay instances which still communicate with the project cache, | ||
| /// but never fetch project configs from upstream. In this configuration data must still be | ||
| /// forwarded even without a project config. | ||
| DummyAllowed, |
There was a problem hiding this comment.
Naming suggestion.
| DummyAllowed, | |
| Dummy, |
3 participants
Introduces a dedicated state in the
ProjectStatefor Proxy Relays which do not fetch project configs from upstream.This makes a few implicit decisions explicit and more obvious. This PR is largely without side-effects. The biggest "hidden" change is that
enabled()now returnsNonefor the new state, but all usages ofenabled()use it to check feature flags, which will always fail even with the previous dummy state.Also taking suggestions for a better name than
DummyAllowed.